Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: include "vitest" in the process name #4191

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

sheremet-va
Copy link
Member

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@stackblitz
Copy link

stackblitz bot commented Sep 27, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Sep 27, 2023

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit fc6330b
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/651467459749ba0008b11979

@sheremet-va
Copy link
Member Author

@AriPerkkio what do you think?

@@ -167,6 +167,8 @@ function normalizeCliOptions(argv: CliOptions): CliOptions {
}

async function start(mode: VitestRunMode, cliFilters: string[], options: CliOptions): Promise<Vitest | undefined> {
process.title = 'node (vitest)'
Copy link
Member Author

@sheremet-va sheremet-va Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bun doesn't support changing title yet, but to be future proof we can do process.title += ' (vitest)'

Copy link
Contributor

@silverwind silverwind Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.title can not be longer than the original title:

https://nodejs.org/api/process.html#processtitle

on Linux and macOS, process.title is limited to the size of the binary name plus the length of the command-line arguments because setting the process.title overwrites the argv memory of the process

So I assume += will likely crash the runtime on affected platforms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how it will do that from the comment you linked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you saying that process.title already fulfills these requirements before reassigning?

Copy link
Member Author

@sheremet-va sheremet-va Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From manual testing it looks like that title cannot be longer than node {filename} - in our case it's probably(?) node vitest.mjs.

When running Vitest, I wasn't able to come up with the name that would not fit in the activity monitor. If you just have a simple file, the limit is reached pretty fast:

process.title = 'node (vitest)'
setTimeout(() => {}, 5000)
node t.mjs

Activity monitor shows:

node (vite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activity monitor shows:

node (vite

Would vitest (node) be better? 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem exists only for this custom file due to the short name. Vitest doesn't have this issue

Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for my debugging workflow. 👍

Without the node prefix it would not show in pgrep node so having that is great. We should also add this to entrypoints of node:child_process, e.g. packages/vitest/src/runtime/child.ts

@silverwind
Copy link
Contributor

silverwind commented Sep 27, 2023

Why not just process.title = 'vitest'? Many process list parsing tools can match that out of the box, but can likely not match node (vitest). Also, put a try-catch around it for unsupporting runtimes.

@sheremet-va
Copy link
Member Author

sheremet-va commented Sep 27, 2023

Why not just process.title = 'vitest'? Many process list parsing tools can match that out of the box, but can likely not match node (vitest). Also, put a try-catch around it for unsupporting runtimes.

Because people look for the "node" process usually. This is why I asked @AriPerkkio opinion here (this change was discussed in a team meeting)

@AriPerkkio
Copy link
Member

AriPerkkio commented Sep 27, 2023

Why not just process.title = 'vitest'?

If your tests spawn external processes you won't see those if you look for vitest. Minimal repro below:

process.title = "vitest";
setTimeout(() => {}, 5000);
$ node repro.mjs

$ pgrep node
> no results

This works:

process.title = "node (vitest)";
setTimeout(() => {}, 5000);
$ node repro.mjs

$ pgrep node
76662       # <--- Yes this is good

$ pgrep vitest
76662          # <-- also here

$ ps $(pgrep node)
  PID   TT  STAT      TIME COMMAND
76662 s008  S+     0:00.04 node (vitest) 

@sheremet-va sheremet-va changed the title feat: include "vitest" in activity monitor feat: include "vitest" in the process name Sep 27, 2023
@sheremet-va sheremet-va merged commit 404c1f4 into vitest-dev:main Sep 28, 2023
14 of 16 checks passed
@sheremet-va sheremet-va deleted the feat/change-process-title branch September 28, 2023 07:12
@silverwind
Copy link
Contributor

silverwind commented Sep 28, 2023

I prefer pidof over pgrep because it's a exact match it won't find it with the current name:

$ node -e 'process.title = "node (vitest)"; setTimeout(() => {}, 60000);'
$ pidof vitest # fails
$ node -e 'process.title = "vitest"; setTimeout(() => {}, 60000);'
$ pidof vitest # works

@silverwind
Copy link
Contributor

Also, what if vitest is run in another runtime like bun or deno? It's wrong to identify as node in such cases.

@sheremet-va
Copy link
Member Author

Also, what if vitest is run in another runtime like bun or deno? It's wrong to identify as node in such cases.

This should not be a problem because:

  1. bun doesn't support process renaming
  2. Deno doesn't support running vitest

@silverwind
Copy link
Contributor

silverwind commented Sep 28, 2023

bun doesn't support process renaming

It will eventually and then you'll have to needlessly change the process name again.

I still think vitest is the ideal process name but if you persist on this silly name, use at least path.basename(process.argv[0]) to make it agnostic to the runtime.

LorenzoBloedow pushed a commit to LorenzoBloedow/vitest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants